@W-20806053 Implement Private Methods#5711
@W-20806053 Implement Private Methods#5711a-chabot wants to merge 32 commits intosalesforce:masterfrom
Conversation
Signed-off-by: a.chabot <a.chabot@salesforce.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: a.chabot <a.chabot@salesforce.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Detect when a user-defined method collides with the reserved __lwc_component_class_internal_private_ prefix by tracking which methods the forward transform renames and verifying the reverse transform only restores those same methods. Throws a descriptive error on mismatch. Made-with: Cursor
…thod transforms Add 10 new tests covering forward-only output verification, combined flags, method ordering, default/destructuring params, empty bodies, same-name coexistence, and intermediate plugin scenarios (body mutation, method injection, near-miss prefix matching). Made-with: Cursor
| Program: { | ||
| enter(path: NodePath<types.Program>, state: LwcBabelPluginPass) { | ||
| const transformedNames = new Set<string>(); | ||
|
|
||
| // Transform private methods BEFORE any other plugin processes them | ||
| path.traverse( | ||
| { | ||
| ClassPrivateMethod( |
There was a problem hiding this comment.
Why do we have a Program visitor that traverses with a ClassPrivateMethod visitor instead of just a top-level ClassPrivateMethod visitor?
Modifying nodes can trigger re-evaluation of visitors. If I understand correctly, this means that the current implementation will do a full traversal looking for ClassPrivateMethod whenever a Program is re-evaluated, even if it's not a changed ClassPrivateMethod that triggered re-evaluation. That seems like not what we want.
There was a problem hiding this comment.
If you switch the top-level ClassPrivateMethod visitor and it causes an infinite loop. The cause of this is that the forward and reverse transforms are both active during the same Babel traversal (Babel merges all plugin visitors into a single pass).
When a top-level ClassPrivateMethod visitor replaces a node with ClassMethod, the reverse transform's ClassMethod visitor immediately fires on the replacement and converts it back to ClassPrivateMethod, which triggers the forward visitor again
Loop: ClassPrivateMethod --> ClassMethod --> ClassPrivateMethod --> ClassMethod --> ....
The Program + path.traverse() pattern avoids this because it's a manual one-shot traversal that completes all forward replacements before Babel's main traversal reaches any class member nodes. By the time the reverse transform's ClassMethod visitor is active, there are no ClassPrivateMethod nodes left to fight over.
You're right that Babel can re-evaluate a Program visitor when descendant nodes are modified. I split out the forward transform to be its own separate plugin, whose only visitor is Program. Since the forward transform itself doesn't modify any Program-level nodes (it only replaces ClassPrivateMethod nodes deep inside the tree), it won't trigger its own re-evaluation.
packages/@lwc/babel-plugin-component/src/private-method-transform.ts
Outdated
Show resolved
Hide resolved
| // Preserve TypeScript annotations and source location when present | ||
| if (node.returnType != null) { | ||
| classMethod.returnType = node.returnType; | ||
| } | ||
| if (node.typeParameters != null) { | ||
| classMethod.typeParameters = node.typeParameters; | ||
| } | ||
| if (node.loc != null) { | ||
| classMethod.loc = node.loc; | ||
| } | ||
| // Preserve TypeScript/ECMAScript modifier flags (excluded from t.classMethod() builder) | ||
| if (node.abstract != null) { | ||
| classMethod.abstract = node.abstract; | ||
| } | ||
| if (node.access != null) { | ||
| classMethod.access = node.access; | ||
| } | ||
| if (node.accessibility != null) { | ||
| classMethod.accessibility = node.accessibility; | ||
| } | ||
| if (node.optional != null) { | ||
| classMethod.optional = node.optional; | ||
| } | ||
| if (node.override != null) { | ||
| classMethod.override = node.override; | ||
| } |
There was a problem hiding this comment.
Is all of this information still required at this point?
There was a problem hiding this comment.
These aren't required by the builder but I do think should be preserved when switching between node types (between ClassMethod and ClassPrivateMethod nodes)
https://babeljs.io/docs/babel-types?utm_source=chatgpt.com#classmethod
packages/@lwc/babel-plugin-component/src/__tests__/private-method-transform.spec.ts
Outdated
Show resolved
Hide resolved
packages/@lwc/babel-plugin-component/src/__tests__/private-method-transform.spec.ts
Show resolved
Hide resolved
packages/@lwc/babel-plugin-component/src/private-method-transform.ts
Outdated
Show resolved
Hide resolved
packages/@lwc/babel-plugin-component/src/reverse-private-method-transform.ts
Outdated
Show resolved
Hide resolved
ekashida
left a comment
There was a problem hiding this comment.
If we are allowing for intermediate broken state between the forward and reverse transforms, I think we should have some end-to-end tests in the form of fixtures to ensure internal usage of these private methods remain intact.
| expect(result!.code).not.toContain('__lwc_component_class_internal_private_'); | ||
| }); | ||
|
|
||
| test('private getters and setters are not transformed', () => { |
There was a problem hiding this comment.
This test doesn't seem to be testing what it's saying it's testing.
There was a problem hiding this comment.
The transform seems to support accessor methods, which I'm personally fine with, but we explicitly said we wouldn't in the RFC.
There was a problem hiding this comment.
Thinking about DX, should we have informative errors for static and non-static private properties and accessor methods? Something like, "Private methods are the only private member type that is currently supported."?
There was a problem hiding this comment.
Gotcha, added LWC1214 to handle this
…served prefix More precise guidance: "cannot start with reserved prefix `__lwc_`" instead of "conflicts with internal naming conventions". Made-with: Cursor
Remove private method example added for testing; not needed in the playground. Made-with: Cursor
Move the constant literal out of the visitor body into a module-level METHOD_KIND constant. Made-with: Cursor
… transforms Deduplicate the repeated if-node.X-!= null property copying into a single helper in utils.ts, used by both the forward and reverse transforms. Made-with: Cursor
…ent plugins Both transforms are now standalone Babel plugins returning PluginObj directly, re-exported from index.ts. The forward transform is no longer bundled inside the main LwcClassTransform Program.enter; instead it runs as a separate plugin before the main plugin in the pipeline. Made-with: Cursor
Add ! to the transformSync() return in each helper function so callers can use result.code instead of result!.code everywhere. Made-with: Cursor
Combine split expect(code).toContain() calls into single assertions that
match the full substring (e.g. 'static async #fetch(url, opts = {})').
Made-with: Cursor
Verify that private fields (#count, #name) are not affected by the private method transform, both alone and alongside private methods. Made-with: Cursor
Validate that private method call sites, private field references in method bodies, and mixed field/method classes all behave correctly through the transform pipeline without leaking prefixed names. Made-with: Cursor
ad63005 to
3ea2b0a
Compare
Add LWC1214 error for unsupported private member types (fields and accessor methods). Update the forward transform to throw informative errors instead of silently passing through. Replace affected unit tests with error-expectation tests. Add fixture-based end-to-end tests that run the full pipeline (forward transform, LWC plugin, class-properties plugin, reverse transform) to verify private methods survive round-trip. Made-with: Cursor
3ea2b0a to
f284247
Compare
Made-with: Cursor
Extend the private method transform to also handle MemberExpression nodes with PrivateName properties (e.g. this.#foo()), not just ClassPrivateMethod definitions. This ensures the intermediate AST is fully consistent—both definitions and call sites use the prefixed public name—so intermediate plugins can process the code correctly. Made-with: Cursor
| MemberExpression(memberPath: NodePath<types.MemberExpression>) { | ||
| const property = memberPath.node.property; | ||
| if (t.isPrivateName(property)) { | ||
| const baseName = (property as types.PrivateName).id.name; | ||
| if (privateMethodBaseNames.has(baseName)) { | ||
| const prefixedName = `${PRIVATE_METHOD_PREFIX}${baseName}`; | ||
| memberPath | ||
| .get('property') | ||
| .replaceWith(t.identifier(prefixedName)); | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Added to transform invocations (forward)
| MemberExpression(path: NodePath<types.MemberExpression>, state: LwcBabelPluginPass) { | ||
| const property = path.node.property; | ||
| if (!t.isIdentifier(property) || !property.name.startsWith(PRIVATE_METHOD_PREFIX)) { | ||
| return; | ||
| } | ||
|
|
||
| const forwardTransformedNames: Set<string> | undefined = ( | ||
| state.file.metadata as any | ||
| )[PRIVATE_METHOD_METADATA_KEY]; | ||
|
|
||
| if (!forwardTransformedNames || !forwardTransformedNames.has(property.name)) { | ||
| return; | ||
| } | ||
|
|
||
| const originalName = property.name.replace(PRIVATE_METHOD_PREFIX, ''); | ||
| path.get('property').replaceWith(t.privateName(t.identifier(originalName))); | ||
| }, |
There was a problem hiding this comment.
Added to transform invocations (reverse)
…ler flag The private method round-trip transform is now only applied when enablePrivateMethods is explicitly set to true in TransformOptions, allowing the feature to be restricted to internal components. Made-with: Cursor
… playground Add enablePrivateMethods to RollupLwcOptions and forward it to the compiler's transformSync call. Enable it in the playground rollup config and add a private method to the counter component for testing. Made-with: Cursor
Remove test private method from counter component and revert playground rollup config back to default lwc() options. Made-with: Cursor
…-flag feat: gate private method transform behind enablePrivateMethods compi…
Remove fixture test files from this branch; they now live on a-chabot/private-methods-fixture-tests. Made-with: Cursor
Remove 16 inline tests from private-method-transform.spec.ts that are now covered by fixture-based tests on the fixture branch. Made-with: Cursor
Remove 13 inline tests from private-method-transform.spec.ts that are now redundant with fixture tests (8 already covered, 5 converted to new fixtures). Add comments to the 10 remaining inline tests explaining why they must stay inline (custom pipelines, forward-only, reverse-only). Made-with: Cursor
Consolidate the individual "Kept inline" comments into a single explanatory note at the top of the file describing why these tests cannot be fixture tests. Made-with: Cursor
Test that cross-class #privateName access is a parse error, and that a spoofed mangled name definition is harmlessly round-tripped back to a class-scoped private method. Made-with: Cursor
Made-with: Cursor
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
This will get replaced with the tests that are in this PR: a-chabot#2
The private-methods fixtures directory only contains .gitkeep on this branch, causing CI to fail with 'No test found in suite'. Skipping the test suite temporarily until the fixture tests from a-chabot/private-method-fixture-tests are merged. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
| return { code }; | ||
| } | ||
|
|
||
| describe.skip('private-methods fixtures', () => { |
There was a problem hiding this comment.
Skipped for now because the fixtures folder is empty but once a-chabot#2 gets merged in, this will get re-enabled
…thods-transform Made-with: Cursor
Details
Link to RFC
Tests PR
Create feature flag in lwc-platform
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item
W-20806053